FEEL variable scope resolution #43

abdul99ahad · 2024-12-16T18:23:57Z

Proposed Changes

List of changes related to scope:

Only resolve result variable globally if no output mapping exists.
Resolve result variable locally if output mapping exists.
Result variable and input have same names, result variable will be available throughout overwriting input variable.
Result variable and output have same names, output variable will be available overwriting result variable.
Input and output have same names, output variable name will overwrite the input variable.
When multiple input or output variables with same name exists within the same process/activity, the later one overwrites all the previous ones.

Visual demo

When script result variable and input with same names coexist, result variable overwrites the input variable.

When same name input/output mapping exists, the later one overwrites the previous one.

You can test the implementation by running this command: npx @bpmn-io/sr bpmn-io/variable-resolver#variable-scoping -l bpmn-io/extract-process-variables#main

Checklist

To ensure you provided everything we need to look at your PR:

Brief textual description of the changes present
Visual demo attached
Steps to try out present, i.e. using the @bpmn-io/sr tool
Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

nikku · 2025-01-02T08:06:30Z

Upstream fix released, this can be moved further.

nikku · 2025-01-07T16:17:48Z

lib/zeebe/util/feelUtility.js

@@ -198,7 +198,7 @@ function getIoExpression(variable, origin) {
    return;
  }

-  const mapping = mappings.find(mapping => mapping.target === variable.name);


I'd apprechiate if we'd group the fix with the actual test that verifies the fix. This way it is easier to understand what we fixed.

This fix corresponds to

When multiple input or output variables with same name exists within the same process/activity, the later one overwrites all the previous ones.

I'll see if the test case doesn't exist for this, I'll add one. Thanks for pointing this out.

nikku · 2025-01-07T16:22:47Z

lib/zeebe/VariableResolver.js

@@ -59,6 +58,12 @@ export default class ZeebeVariableResolver extends BaseVariableResolver {
      // Filter all pre-defined variables
      return !namesToFilter.includes(v.name);
    });
+
+    // keep only unique names
+    const uniqueVariables = new Map();


This can be simplified in some ways (I think):

// use min-dash uniqueBy utility const uniqueVariables = uniqueBy('name', filteredVariables.reverse());

const uniqueVariables = Object.values( filteredVariables.reduce((uniqueVariables, v) => { uniqueVariables[v.name] = v; return uniqueVariables; }) );

Maybe it even makes sense to do the filtering in the previous block?

Implemented. The variables are now filtered distinctly using uniqueBy from min-dash in the base VariableResolver.

nikku · 2025-01-07T18:46:11Z

lib/zeebe/util/feelUtility.js

@@ -198,7 +198,7 @@ function getIoExpression(variable, origin) {
    return;
  }

-  const mapping = mappings.find(mapping => mapping.target === variable.name);


We could also search mappings from last to first:

mappings.reverse().find(...);

Is there any particular reason we're sticking to find(...) and avoid filter(...).pop() ?

Find aborts early, while filter goes through the whole list. But in the end it is a matter of taste.

pop() is also not side-effect free, so it is not fully functional, and may cause (in some situations) issues.

nikku · 2025-01-08T12:07:01Z

lib/zeebe/util/feelUtility.js

@@ -198,7 +198,7 @@ function getIoExpression(variable, origin) {
    return;
  }

-  const mapping = mappings.find(mapping => mapping.target === variable.name);


Find aborts early, while filter goes through the whole list. But in the end it is a matter of taste.

pop() is also not side-effect free, so it is not fully functional, and may cause (in some situations) issues.

nikku · 2025-01-08T12:09:00Z

lib/zeebe/VariableResolver.js

-
-      namesToFilter.push(...outputsToFilter);
-    }
+    const namesToFilter = getElementNamesToRemove(moddleElement, inputOutput);


Same with other commit in this PR, what problem does this fix solve? I'd apprechiate if we'd pack implementation + test case together.

This isn't a fix, just a refactor to clean up the code into a separate function. I'll make sure the commits are clear for reviewers.

nikku

@abdul99ahad Thanks for doing this additional round. I still find it a little bit hard to review your PR, because some "FIX" commits still don't contain tests for the actual bugs fixed.

For any reasonably complex topic (such as this one) we need rigid test coverage. I advice you to adopt the practice to pack fix + test into one commit. This helps reviewers to trace what you were doing, and what exactly the expected effect is.

abdul99ahad · 2025-01-08T13:58:23Z

@abdul99ahad Thanks for doing this additional round. I still find it a little bit hard to review your PR, because some "FIX" commits still don't contain tests for the actual bugs fixed.

I advice you to adopt the practice to pack fix + test into one commit. This helps reviewers to trace what you were doing, and what exactly the expected effect is.

Thanks for pointing this out, Nico. I’ll make sure to include tests alongside fixes in a single commit for better traceability in the future. For now, I’ve separated the refactor into its own commit to keep things clearer.

nikku · 2025-01-09T09:17:17Z

Ok, I'll see what I can do, and update this PR with co-location. Will ping you to check back with you, if I grouped things appropriately.

Prioritize variable resolution as follows: - If both result and output variables exist, prioritize output. - If both result and input variables exist, prioritize result.

abdul99ahad · 2025-01-11T14:36:18Z

Ok, I'll see what I can do, and update this PR with co-location. Will ping you to check back with you, if I grouped things appropriately.

I have grouped the fixes and tests. 🏅

bpmn-io-tasks bot added the needs review Review pending label Dec 16, 2024

bpmn-io-tasks bot assigned abdul99ahad Dec 16, 2024

This comment was marked as outdated.

Sign in to view

abdul99ahad requested review from nikku and barmac December 16, 2024 18:24

abdul99ahad marked this pull request as draft December 16, 2024 18:28

bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Dec 16, 2024

abdul99ahad force-pushed the variable-scoping branch 4 times, most recently from 7d79e35 to b55efe3 Compare January 7, 2025 11:05

abdul99ahad marked this pull request as ready for review January 7, 2025 11:18

bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 7, 2025

nikku reviewed Jan 7, 2025

View reviewed changes

abdul99ahad force-pushed the variable-scoping branch 5 times, most recently from e87fb60 to 6e3dca3 Compare January 8, 2025 10:33

abdul99ahad requested a review from nikku January 8, 2025 10:35

nikku requested changes Jan 8, 2025

View reviewed changes

bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Jan 8, 2025

nikku reviewed Jan 8, 2025

View reviewed changes

abdul99ahad force-pushed the variable-scoping branch from 6e3dca3 to f33b294 Compare January 8, 2025 13:52

abdul99ahad added 5 commits January 11, 2025 18:56

chore(refactor): remove element after current def into separate function

40776f1

test: modify scope.bpmn xml

b5ce7bc

chore(deps): update extract-process-variables deps

3f4ad4a

fix: resolve scope priority for variables with the same name

7675d96

Prioritize variable resolution as follows: - If both result and output variables exist, prioritize output. - If both result and input variables exist, prioritize result.

fix: latest io mapping overwrite when duplicates

fd86b60

abdul99ahad force-pushed the variable-scoping branch from f33b294 to fd86b60 Compare January 11, 2025 14:31

abdul99ahad requested a review from nikku January 11, 2025 14:36

bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEEL variable scope resolution #43

FEEL variable scope resolution #43

abdul99ahad commented Dec 16, 2024 •

edited

Loading

This comment was marked as outdated.

nikku commented Jan 2, 2025

nikku Jan 7, 2025

abdul99ahad Jan 7, 2025

nikku Jan 7, 2025

abdul99ahad Jan 8, 2025

nikku Jan 7, 2025

abdul99ahad Jan 8, 2025

nikku Jan 8, 2025

nikku Jan 8, 2025

nikku Jan 8, 2025

abdul99ahad Jan 8, 2025

nikku left a comment

abdul99ahad commented Jan 8, 2025

nikku commented Jan 9, 2025

abdul99ahad commented Jan 11, 2025

FEEL variable scope resolution #43

Are you sure you want to change the base?

FEEL variable scope resolution #43

Conversation

abdul99ahad commented Dec 16, 2024 • edited Loading

Proposed Changes

Visual demo

Checklist

This comment was marked as outdated.

nikku commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikku left a comment

Choose a reason for hiding this comment

abdul99ahad commented Jan 8, 2025

nikku commented Jan 9, 2025

abdul99ahad commented Jan 11, 2025

abdul99ahad commented Dec 16, 2024 •

edited

Loading